I'm Bonk, and I've done a quick review of your PR.

This PR adds isExceptionJsgError to detect user-generated jsg.Error exceptions in alarm error descriptions, ensuring they count against retry limits.

Findings posted as inline review comments (by severity):

1. [MEDIUM] The header comment in exception.h:160 says the function strips "remote." prefixes, but the implementation only strips "remote exception: " -- not the "remote." prefix that annotateBroken() can produce. Suggested a comment fix.

2. [MEDIUM] The hand-rolled prefix-stripping loop in exception.c++:174-184 could be replaced by reusing tunneledErrorType(), which already handles "remote.", "expected ", and "broken." prefixes correctly. Suggested a one-liner replacement.

3. [LOW] No unit tests for isExceptionJsgError -- the function does non-trivial string parsing and would benefit from test coverage.

github run

The code is consistent. Both error-handling paths (the exception catch at line 540 and the output lock failure at line 609) use the same pattern with isJsgError.

Let me verify one more subtle thing — the exception-test.c++ test needs to be in the BUILD.bazel deps correctly. The glob at line 296-310 picks up *-test.c++ excluding specific files, and exception-test.c++ is not in the exclude list. The deps are [":jsg"]. Let me check that exception.h is reachable through :jsg:

Looking at line 18-19 of BUILD.bazel, :jsg depends on :exception. The :exception target (line 215-223) has exception.c++ as srcs and exception.h as hdrs. The test #include "exception.h" will work via the transitive dep.

However, there's an issue: the test only uses symbols from :exception (just isExceptionJsgError and isExceptionFromInputGateBroken), but the glob-based rule links it against :jsg which pulls in V8 and everything else. This is wasteful but functional — and it's the existing pattern for the glob rule, so it's fine. Not worth flagging.

Let me now check if there are any remaining issues I should flag. The comment at line 176 says "starts with 'Error'" but the code checks "Error:"_kj — this is a minor inconsistency in the comment but not a bug. Actually, the comment is fine: "starts with 'Error'" is informal, and the code correctly checks "Error:" to distinguish from "TypeError:", "RangeError:", etc.

Let me think about whether this change needs a compat flag or autogate. This is a behavioral change in how alarm retries are classified — user-thrown jsg.Error exceptions will now count against retry limits when they previously didn't. However:

1. This is a bug fix — user errors should have been counting against limits already.
2. The change is in server-side retry behavior, not in the JavaScript API surface.
3. The effect is that alarms with user errors stop retrying sooner (a good thing — prevents infinite retry loops for user bugs).

This doesn't need a compat flag because it's fixing incorrect server-side behavior, not changing the JS API contract. It also doesn't seem risky enough to need an autogate since it's narrowly scoped to jsg.Error specifically.

All my previous review findings have been addressed. The implementation is clean, uses tunneledErrorType() as I suggested, has comprehensive tests, and the logic is consistent across both error paths. There are no new actionable issues.

LGTM

github run

Everything looks consistent and correct. The implementation is clean, the test coverage is comprehensive, and the previous review findings have been addressed. The approach of not using tunneledErrorType() is actually the correct design choice given the need for an allowlist of broken prefixes.

LGTM

github run

The implementation is clean and correct. The prefix stripping order is correct: first "remote exception: " (KJ RPC tunneling), then "remote." (annotateBroken), then one allowlisted broken.* prefix.

One subtle observation: isExceptionFromInputGateBroken (line 168-171) does NOT strip "remote." prefixes - it only strips "remote exception: ". This means remote.broken.inputGateBroken; jsg.Error: would NOT be caught by isExceptionFromInputGateBroken but WOULD be caught by isExceptionJsgError (since it strips "remote." then sees broken. which isn't in the allowlist, so it doesn't match). Wait - actually no. After stripping "remote.", we'd have broken.inputGateBroken; jsg.Error: which is NOT in the allowlist, so isExceptionJsgError returns false. Good - the test at line 46-48 confirms this.

But wait - there's actually an edge case here. If someone has remote.broken.outputGateBroken; jsg.Error:, isExceptionJsgError strips "remote." to get broken.outputGateBroken; jsg.Error: which matches. But isExceptionFromInputGateBroken can't handle remote.broken.inputGateBroken; ... - the test at line 116-120 documents this gap. However this is a pre-existing issue with isExceptionFromInputGateBroken, not introduced by this PR.

I've completed my analysis. The PR is well-structured, addresses the previous review feedback, has comprehensive tests, and the behavioral changes are correct. The jsg.Error to jsg-internal.Error changes in actor-cache.c++ and worker.c++ are proper defensive hardening. There are no actionable issues.

LGTM

github run

I'm Bonk, and I've done a quick review of your PR.

This PR shifts alarm error classification from string-parsing a dedicated isExceptionJsgError function to tagging exceptions with EXCEPTION_IS_USER_ERROR at construction sites and using inline startsWith("jsg."_kj) as a fallback.

Findings posted as inline review comments (by severity):

1. [MEDIUM] The two error paths handle the "remote exception: " prefix inconsistently — the first path (line 577) checks the raw description, while the second path (line 622) strips the prefix first via stripRemoteExceptionPrefix().

2. [MEDIUM] kentonv's feedback from the previous revision about reusing tunneledErrorType() instead of writing new parsing code is still not addressed. The inline startsWith("jsg."_kj) checks are still ad-hoc string parsing. Both inline comments include a concrete tunneledErrorType()-based suggestion.

3. [LOW] The user-error classification logic (isUserError || isInputGateBrokenByUser || isPlainJsgError) is duplicated across both error paths, making it easy for them to diverge (as they already have with the prefix stripping inconsistency).

github run

LGTM

github run

Changed how the user error classification was done. This version will consider user errors plain jsg.* errors, or broken.* errors that were tagged with EXCEPTION_IS_USER_ERROR.
This should be good to review